Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP refact(table): decouple buffer from table-view by moving paging up to model #70

Merged
merged 13 commits into from
Jun 14, 2017
Merged

WIP refact(table): decouple buffer from table-view by moving paging up to model #70

merged 13 commits into from
Jun 14, 2017

Conversation

ankostis
Copy link
Contributor

@ankostis ankostis commented Jun 14, 2017 •
edited
Loading

I want to add a new feature that will display pandas dataframes(see #35), but I had problems factoring out a clean table model that exposes the dataframe cells. In the process I realized that the table vitables.vttables classes LeafView/LeafModel/Buffer were highly coupled, without clear responsibilities.
This PR restructures model & buffer according to these guidelines:

  • Encapsulate buffer so as to be accessed only by model by dropping or moving buffer's paging fields UP into model:
    • Move start/nrows/chunk_size attributes up, from buffer-->model.
    • Merge model.chunk_size with nrows in model - they served the same purpose
    • Move up & merge buffer.getReadParameters()-->model.loadData().
  • Minor speedups (e.g. don't fetch cell-data if not display-role).
  • Stop using np.int64 for long tables - python ints are just as fine.
  • PEP8 & moderate renaming of variables (e.g. data_set-->leaf).
  • Apply (partially ) good practices on logging- reuse module loggers on new objects , don't set log-level in code.

WIP:

  • I started gradually implementing the refactorings, hopefully separated in small chunks.
  • The move/merge of variables from buffer --> model had to be done in one commit.
  • There are some more methods to move down from model to buffer regarding total-columns.
  • What is really left is testing, ensuring plugins work correctly, and documenting the new setup in the class docstrings and possibly in the docs of the project.
  • Any help, particularly for the testing would be welcomed.

(when it is finished, the 'WIP' has to be removed from this PR's title.)

ankostis added 7 commits June 13, 2017 22:39
@ankostis
refact(buffer): needless np.int64 instead of regular python ints
dc13b1f 
+ Python's ints have arbitrary digits, are not just 32bit.
@ankostis
refact(buffer): don't re-offset with buffer.start when reading cells
4779c3b 
+ The `buffer.start` offset was added by models/timeseries to describe
coordinates against the full table;
buffer internally was substracting it back.
+ Change reduces a bit coupling to internals of buffer (Demeter law).
@ankostis
fix(buffer): speedup table readability-check
8a4e0d0 
+ Reading a full chunk (1000 rows) from a compressed HDF5 file would be
a waste of CPU.
+ Also decouple readability-check from chunk-size/start value
@ankostis
refact(table): do not load cell-values if not display QtRole
1991ca2 
+ Speedup model by avoiding useless data-loading.
@ankostis
refact(table): rename data_source-->leaf
5f6b563
@ankostis
feat(table): drop minor forgotten enum_indices method
63f39f0
@ankostis
refact(scroll): move slider max-value calculation in scrollbar-class
7c8ca9f
@ankostis
Copy link
Contributor Author

@uvemas I would also like to consolidate classes a bit in fewer modules:

  • put Buffer inside vttables.leaf_model module, since it is now completely encapsulated, and
  • put both LeafDelegate & ScrollBar inside vttables.leaf_view module.

It does make sense, for instance, when a logger writes a message, you probably care if it is a view or model, but not if it is a scrollbar or a delegate. Python tradition diverts from java's one class, one file to keep things terse.

Would that be ok?

ankostis added 6 commits June 14, 2017 15:27
@ankostis
refact(buffer): MOVE PAGING from buffer-->model to DECOUPLE view
2ef09d4 
+ Move start/nrows/chunk_size attributes up, from buffer-->model.
+ Drop or move buffer-fields, to decouple it from model & view (ie. to
facilitate replacing it with a DataFrame).
+ Move & merge buffer.getReadParameters()-->model.loadData().
+ DROP `model.chunk_size`, it is identical to `nrows`.
- Indexing errors in `buffer.getCell()` don't report `start` anymore :-(
@ankostis
style(tabview): pep8
254264f
@ankostis
style(tabmodel): reodred methods
d5dc8a6
@ankostis
style(tabview): shorter names for model-item attributes
f2679e3
@ankostis
refact(table): encapsulate buf.getCell() & improve logging
4ad73cc 
+ Centralize error-reporting for cell indexing-errors.
+ Logging:
  + Restore start offset-logging on cell indexing errors.
  + Class-logger-->module-var, not to waste loggers with each new
object.
  + Stop specifying log-levels (must be done to all loggers).
@ankostis
feat(datasheet): don't zoom-cells if empty
ae05d19
@ankostis ankostis mentioned this pull request Jun 14, 2017
Merged
@uvemas uvemas merged commit da09b75 into uvemas:master Jun 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ankostis @uvemas